feat(snowflake): extend Entity class to support snowflakes #56795
Conversation
miaulalala
requested review from
CarlSchwan,
nickvergessen,
provokateurin and
skjnldsv
miaulalala
requested review from
ArtificialOwl and
icewind1991
and removed request for
a team
|
We should rename IGenerator and IDecoder to ISnowflakeGenerator and ISnowflakeDecoder, otherwise after a use all code is ambiguous. |
Added |
Altahrim
left a comment
Altahrim
left a comment
There was a problem hiding this comment.
If we introduce a Snowflake DTO, I think Decoder should return it directly
|
Would it make sense to cache the decoded snowflake ID? Seems like there's quite a few expensive calculations going on. |
|
We can still add a cache later if needed but decoding on 64 bits mainly involve binary operations, it should be fast. |
miaulalala
added
3. to review
Altahrim
left a comment
Altahrim
left a comment
There was a problem hiding this comment.
With same comment than Côme :)
provokateurin
left a comment
provokateurin
left a comment
There was a problem hiding this comment.
Some small changes would be nice. Also I would prefer if the changes were done in multiple commits. I also noticed the name of the DI variable is inconsistent, maybe you can fix that as well.
- [ ] needs nextcloud/server#56795 - [ ] needs #16508 Skipping CI for now, amend the commit message and remove the skip-ci param when ready to rebase and merge [skip-ci] Signed-off-by: Anna Larch <[email protected]>
miaulalala
force-pushed
the
feat/noid/extend-entity-to-be-snoflake-aware
branch
from
75a21de to
8d35e95
Compare
- [ ] needs nextcloud/server#56795 - [ ] needs #16508 Skipping CI for now, amend the commit message and remove the skip-ci param when ready to rebase and merge [skip ci] Signed-off-by: Anna Larch <[email protected]>
miaulalala
force-pushed
the
feat/noid/extend-entity-to-be-snoflake-aware
branch
from
ba2f797 to
67b168a
Compare
CarlSchwan
force-pushed
the
feat/noid/extend-entity-to-be-snoflake-aware
branch
3 times, most recently
from
29d972b to
4238c1e
Compare
And introduce the Snowflake DTO Signed-off-by: Anna Larch <[email protected]>
Signed-off-by: Anna Larch <[email protected]>
Signed-off-by: Anna Larch <[email protected]>
- Rename setId() -> generateId() in SnowflakeAwareEntity Signed-off-by: Carl Schwan <[email protected]>
CarlSchwan
force-pushed
the
feat/noid/extend-entity-to-be-snoflake-aware
branch
from
4238c1e to
7c1a8a4
Compare
Otherwise this breaks some existing code, in particular PublicKeyToken Signed-off-by: Carl Schwan <[email protected]>
- [ ] needs nextcloud/server#56795 - [x] needs #16508 Skipping CI for now, amend the commit message and remove the skip-ci param when ready to rebase and merge [skip ci] Signed-off-by: Anna Larch <[email protected]> Signed-off-by: Carl Schwan <[email protected]>
- [x] needs nextcloud/server#56795 - [x] needs #16508 Signed-off-by: Anna Larch <[email protected]> Signed-off-by: Carl Schwan <[email protected]>
| /** @var int $id */ | ||
| public $id = null; | ||
|
|
||
| public int|string|null $id = null; |
There was a problem hiding this comment.
@miaulalala @CarlSchwan Is this really needed? This is breaking, as now any code accessing this property need to adapt to support all of those types.
There was a problem hiding this comment.
There were some more last polishing PRs around the time of your comment, can you check latest ocp package?
There was a problem hiding this comment.
The typing is still the same on latest master
RFC: extending the Entity class to support snowflake IDs automagically
Good ides - yes / no?
Benefits:
SnowflakeAwareEntityinstead of the regularEntitycreatedAt,setId, and anything else we would like to offer separatelyDrawbacks:
TODO
Checklist
3. to review, feature component)stable32)